Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only set/save the inverse on a HABTM if the inverse is also HABTM #757

Merged
merged 1 commit into from
Apr 7, 2015

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Apr 7, 2015

previously we were getting:

     ActiveFedora::UnknownAttributeError:
            Item does not have an attribute `item_id'

if the inverse was a has_many.

previously we were getting:

```
     ActiveFedora::UnknownAttributeError:
            Item does not have an attribute `item_id'
```

if the inverse was a has_many.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 93.03% when pulling 085d221 on fix_habtm into b156f54 on master.

@cjcolvar
Copy link
Member

cjcolvar commented Apr 7, 2015

I don't quite understand all that is going on here, but it looks like activerecord doesn't do anything with inverse_of on HABTM relationships (https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/associations.rb#L1641-L1678). If we're trying to mostly track activerecord's API, do we even need this?

@jcoyne
Copy link
Member Author

jcoyne commented Apr 7, 2015

@cjcolvar we need this because out implementation of HABTM is substantially different from ARs implmentation. In ActiveFedora, we use RDF which can have multiple outgoing assertions vs ActiveRecord using a join_table. This commit adds no new features, it merely prevents a bug when the inverse is a has_many instead of another HABTM.

@dchandekstark
Copy link
Member

Wouldn't it be incorrect for the inverse of a HABTM to be a has_many?

@jcoyne
Copy link
Member Author

jcoyne commented Apr 7, 2015

@dchandekstark not in ActiveFedora. The inverse of a HABTM may be a has_many, which is to say the relationships are only outbound, or another HABTM, which means the relationships are bi-directional. A HABTM may also be without an inverse relationship.

@cjcolvar
Copy link
Member

cjcolvar commented Apr 7, 2015

👍

cjcolvar added a commit that referenced this pull request Apr 7, 2015
Only set/save the inverse on a HABTM if the inverse is also HABTM
@cjcolvar cjcolvar merged commit fbfd541 into master Apr 7, 2015
@cjcolvar cjcolvar deleted the fix_habtm branch April 7, 2015 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants